Skip to content

Drastically speed up creating rejudgings by using direct queries #2648

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

nickygerritsen
Copy link
Member

On my laptop creating a rejudging for the whole of WF46 went from 807 seconds to 16 seconds, a 50x improvement.

On my laptop creating a rejudging for the whole of WF46 went from 807 seconds to 16 seconds, a 50x improvement.
@thijskh
Copy link
Member

thijskh commented Aug 12, 2024

May I be the devil's advocate and ask why we just not always use direct queries?

@nickygerritsen
Copy link
Member Author

May I be the devil's advocate and ask why we just not always use direct queries?

For me Doctrine has many advantages, amongst which:

  • A better DX (developer experience) since you have models to work with.
  • Better integration with Symfony (might be considered DX as well).
  • Automatic detection of what changed and what queries to run (this is what made this particular thing slow since we have many changes).
  • Automatic events for validation, etc.

Having said that, there are clear drawbacks, where speed is the biggest one. For normal pages, and for select queries, this will not be a big issue. But for pages where we do many many queries, it becomes slow.

I wouldn't want to not use it anywhere, since I don't feed DOMjudge is slow in general. However, for controllers that do many queries or are used very often it is worth the investigation to see if we can speed them up with not using Doctrine.

@eldering
Copy link
Member

I would argue that a drawback is that even though Doctrine might indeed simplify things and improve DX (not so much the case for me personally), another downside is the added complexity once you move away from standard, simple usage.

We already have a number of places in code where weird hacks are in place. I don't recall all the precise places, but I remember we had some weird stuff in the event handling code, but for example also https://github.com/DOMjudge/domjudge/blob/main/webapp/src/Doctrine/ExternalIdAssigner.php: there's a whole bunch of added complexity on top of simple SQL, which actually start to make the code more difficult to understand, and act at a distance, so naively looking at the code it is not clear what's going on under the hood.

@nickygerritsen
Copy link
Member Author

I wonder for that specific case, where we want to assign some field to all entities we insert in the database, how you would do that when not using events, an ORM, or a framework. Call a function in every place where you insert (which is prone to forgetting it), create a helper method that does the insert (which we could do instead of this with Doctrine as well but it means changing many, many places to use that helper) or something else?
I'm not saying this is the best way to do it or it is the most clear way, but I'm not sure what is. If there is a better way, I'm all for it.
I still remember the "good" old days with lib.db.php and its logic and I really disliked that, for me that was way less easy to use than something like Doctrine in 95% of the cases. But this is of course my opinion and you will have a different one, so maybe we can find some middle ground?

@eldering
Copy link
Member

I wonder for that specific case, where we want to assign some field to all entities we insert in the database, how you would do that when not using events, an ORM, or a framework. Call a function in every place where you insert (which is prone to forgetting it), create a helper method that does the insert (which we could do instead of this with Doctrine as well but it means changing many, many places to use that helper) or something else? I'm not saying this is the best way to do it or it is the most clear way, but I'm not sure what is. If there is a better way, I'm all for it. I still remember the "good" old days with lib.db.php and its logic and I really disliked that, for me that was way less easy to use than something like Doctrine in 95% of the cases. But this is of course my opinion and you will have a different one, so maybe we can find some middle ground?

I understand your point, and I wouldn't mind this if it was only the 95% of cases. The problem I feel is the other 5% and especially the complexity overhead that gives. I'm not sure a discussion about this in a PR will be very helpful, so maybe better to look at this during a hackathon or so.

@nickygerritsen
Copy link
Member Author

Agreed. Let's look at this PR without this huge discussion, since I think this is an improvement regardless

@@ -99,43 +100,62 @@ public function createRejudging(
}


$this->em->wrapInTransaction(function () use (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check that this one was also slow?

If I understand the change correctly we still do a transaction but would loose the flush to the entitymanager, wouldn't we need the flush after the last transaction. Wouldn't we want all those transactions in 1 go?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made it go from ~3x to ~50x speed improvement. The wrap does the exact same thing AND a flush. We don't need the flush if we don't use Doctrine and only direct queries, which we now do.

Doing one transaction might be faster but we also lock the database for some operations for the duration of the transaction, so we better keep it short. I don't think it will make a very big difference anyway.

Copy link
Member

@vmcj vmcj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and as we need this for the WFs we should merge it and have the discussion on doctrine in person.

@nickygerritsen nickygerritsen added this pull request to the merge queue Sep 5, 2024
Merged via the queue into DOMjudge:main with commit e6406f8 Sep 5, 2024
23 of 26 checks passed
@nickygerritsen nickygerritsen deleted the rejudging-create-use-direct-queries branch September 5, 2024 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants